Add details-toggle Catalyst element to improve accessibility of Details component
#3292
+281
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: ba55e1d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ktravers
force-pushed
the
ktravers/upstream-details-element
branch
from
b368ffc to
5af1c2b
Compare
ktravers
force-pushed
the
ktravers/upstream-details-element
branch
from
5af1c2b to
ca0a5eb
Compare
ktravers
force-pushed
the
ktravers/upstream-details-element
branch
from
e222165 to
66761f1
Compare
|
|
Taking this back to draft while I investigate visual differences. |
camertron
requested changes
Jan 31, 2025
Comment on lines
+111
to
+116
| assert_selector("details.dropdown") do | ||
| assert_selector("summary.btn", text: "Button") | ||
| assert_selector("summary.btn[aria-label='Open me']") | ||
| assert_selector("summary[data-aria-label-open='Close me']") | ||
| assert_selector("summary[data-aria-label-closed='Open me']") | ||
| end |
There was a problem hiding this comment.
I think assert_selector with an implicit receiver searches the entire page. Calling it on the yielded element instead should have the nesting effect you're after:
Suggested change
| assert_selector("details.dropdown") do | |
| assert_selector("summary.btn", text: "Button") | |
| assert_selector("summary.btn[aria-label='Open me']") | |
| assert_selector("summary[data-aria-label-open='Close me']") | |
| assert_selector("summary[data-aria-label-closed='Open me']") | |
| end | |
| assert_selector("details.dropdown") do |dropdown| | |
| dropdown.assert_selector("summary.btn", text: "Button") | |
| dropdown.assert_selector("summary.btn[aria-label='Open me']") | |
| dropdown.assert_selector("summary[data-aria-label-open='Close me']") | |
| dropdown.assert_selector("summary[data-aria-label-closed='Open me']") | |
| end |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
This PR adds a new Catalyst element,
details-toggle, to the Details component in order to ensure that when a Details component is toggled open/closed, its aria attributes are updated as expected by default.Previously,
aria-expandedandaria-labelattributes were not included at all, which made it difficult for screen reader users interact with a rendered Details component, given there was no indication what would happen when the component was activated.Note
For GitHub staff reviewers: these changes have been upstreamed from dotcom. See https://github.com/github/github/pull/357658.
Screenshots
Screen.recording.Details.component.aria.attribute.changes.mov
Integration
Yes, but only for
github/github. When this version of@primer/view-componentsis released, we can deprecate the Catalyst element added in https://github.com/github/github/pull/357658. Note that this change isn't technically required, in that nothing will break if we delay or even skip removing the gh/gh component. It's purely code cleanup, removing duped/unnecessary code to improve general maintainability.List the issues that this change affects.
Related to https://github.com/github/accessibility-audits/issues/10012
Risk Assessment
What approach did you choose and why?
I followed existing conventions for this codebase and created a Catalyst component for updating the aria attributes when the details component is toggled open/closed.
Re: visual changes -- These are intentional. They're present because I added reasonable default aria label values to the
Primer::Alpha::Dropdowncomponent, which rendersPrimer::Beta::Details. So Dropdown will also benefit from these accessibility improvements.Anything you want to highlight for special attention from reviewers?
Nothing in particular.
Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.